Allow more complex GC policies #5079
Conversation
There was a problem hiding this comment.
SGTM, but which setting should take priority? I guess if I have keepBytes set to 20GB but only 10GB free space then it should still try to use up to 20 and fail if running out of space. Otherwise, if you accidentally run low on space because of some other tool, buildkit will just start to perform very poorly but there is no indication of why that is.
|
IMO, I think both settings should be followed - I think it would be quite useful and powerful to allow pruning at both constraints. e.g. I might want to have Ideally, if we took this approach, we could modify the defaults for storage, and have I'd argue that the performance metrics should be surfaced around progress/etc, docker desktop, dagger cloud, etc - this kind of poor performance already happens today on tooling like docker desktop when (naively) starting a docker container driver manually - the default disk space is small, and the 10% default is not great for large builds. |
I guess then we need min-storage, max-storage and free. We can keep the existing storage limit as one but need to reevaluate the defaults. |
|
What would I think we would just need |
|
min-storage - amount BuildKit is always allowed to consume, even if free space constraint would say it should start deleting things. To make sure that BuildKit has some reserved storage that it can use for cache even if other apps are misbehaving. |
jedevc
force-pushed
the
free-bytes-gc
branch
from
0a80cf5
to
14c9297
Compare
|
Refactored into You can configure all of them in each policy, which I've done - I've also added some tests for manually verifying what happens when everything gets set together. |
jedevc
force-pushed
the
free-bytes-gc
branch
from
14c9297
to
e7465d5
Compare
|
@tonistiigi @crazy-max any thoughts on this new approach? |
|
|
||
| // if we need to free up space, then decrease to that | ||
| if excess := opt.Free - dstat.Free; excess > 0 { |
There was a problem hiding this comment.
At least in Windows it looks like dstat can be empty. In that case we should not use the values from it.
There was a problem hiding this comment.
Oops, that's a typo - dstat should never be empty, the error should be propagated like in the original code.
There was a problem hiding this comment.
Previously we just logged errors from that function (not sure if #5079 (comment) has been addressed yet). I'm not sure how likely this is but can we make it so that prune only errors if GetDiskStat fails and Free is set. For other deletions I think it could just continue even if the GetDiskStat doesn't give good result.
There was a problem hiding this comment.
Ah I see, I've adjusted to do this now.
|
Btw, not releated to this PR (can fix it in separate) but noticed in CI logs. Looks like if |
jedevc
force-pushed
the
free-bytes-gc
branch
2 times, most recently
from
88e7190
to
99849fa
Compare
Signed-off-by: Justin Chadwell <[email protected]>
We shouldn't use the cachemount root, we should actually properly use the worker's specified root which is propagated from the config. Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
free-bytes-gc
branch
from
99849fa
to
ac7caa8
Compare
We've had some issues with users having difficulty understanding how the cache policy works in dagger, typically run in container environments. Additionally, I've also had issues explaining to users how to configure this in buildkit. One of the problems is with setting a reasonable default - for example, on a development machine, where buildkit has access to the full disk, setting 10% max disk is totally reasonable. But in docker desktop, where you might expect to run a lot of builds, that same 10% max disk is completely inappropriate and far too small. In GitHub actions, we'd also want to use much more of the available space.
Actually, a useful setting would be to be able to configure at how much free disk space the GC should kick in, rather than the consumed disk space. For example, I might prefer to configure the GC to kick in when there is less that 20% total disk space free. This new metric also takes into account other applications that might consume disk space as well (now I can configure buildkit without needing to be aware of what else is going to be taking up lots of space), and make the most of the space on the machine.
This PR replaces the existing
keepBytessettings with 3 new toggles:maxStorage- how much disk this policy is allowed to consumeminStorage- how much disk this policy is always allowed to consume (even when constrained byfree)free- how much disk should be left free